fix(security): patch path traversal and symlink in plugin upload (#719) - #614
fix(security): patch path traversal and symlink in plugin upload (#719)#614tri2510 wants to merge 5 commits into
Conversation
Fixes three vulnerabilities in POST /v2/plugin/upload/:slug:
1. Path traversal via slug (CWE-22): slug was validated only as
Joi.string().required(), allowing URL-encoded ../ sequences to
extract the zip into arbitrary directories (e.g. backend/src/ → RCE).
- Apply the existing slug custom validator (rejects non-slug chars)
- Add path.resolve containment check in the controller (defense in depth)
2. Symlink-based arbitrary file read (CWE-59): spawn('unzip') recreated
symbolic links, and express.static followed them, allowing read
access to any file (e.g. .env containing JWT_SECRET).
- Replace spawn('unzip') with safe yauzl-based extraction that rejects
symlink entries, absolute paths, and ../ in entry names
- Add dotfiles: 'ignore' to express.static mounts for /plugin
3. Missing authorization (CWE-862): the admin checkPermission guard
was commented out, and the ownership check ran after extraction.
- Move ownership check before extraction so files are never written
for unauthorized users
- Remove commented-out checkPermission and redundant auth() from route
(auth() already applied via router.use(auth()) at line 26)
NhanLuongBGSV
left a comment
NhanLuongBGSV
left a comment
There was a problem hiding this comment.
Review — fix(security): patch path traversal and symlink in plugin upload (#719)
Overall: Approve with minor comments. The three reported vulnerabilities are correctly and soundly addressed. The remaining issues are minor (cleanup, scope-creep, test coverage) and don't block merge.
The three fixes are correct
1. Path traversal via slug (CWE-22) — fixed ✅
Joi.string().required().custom(slug)is applied touploadInternal.params.slug.slugis already imported in the validation file (custom.validation.js), andslugify(value) !== valuerejects encoded../sequences after Express decodes them (..%2F..%2Fsrc→../../src→slugifystrips tosrc→ mismatch → 400). The validator runs beforeupload.single('file')in the route, so a bad slug is rejected before multer writes a temp file.- Defense-in-depth
path.resolve(pluginPath)containment check in the controller is valid:PLUGIN_DIR = path.join(__dirname, '../../static/plugin')is normalized bypath.join(it resolves..), so thestartsWith(PLUGIN_DIR + path.sep)check behaves correctly — no false positive for legitimate slugs.
2. Symlink arbitrary file read (CWE-59) — fixed ✅
- Replacing
spawn('unzip')with manualyauzlextraction is the real fix: yauzl never recreates symlinks as actual symlinks — a symlink entry's content (the target path) would be written as a text file, which is harmless. The Unix-mode check (0o120000) is a correct additional guard. Path containment + absolute/..rejection +dotfiles: 'ignore'on theexpress.staticmounts close the read vector. Solid.
3. Missing authorization (CWE-862) — fixed ✅
- Ownership check (
getPluginBySlug→created_bycomparison) is moved before extraction, so no files are written for unauthorized users. Removing the redundantauth()is correct —router.use(auth())at line 26 already authenticates this route. Removing the commented-outcheckPermissionchanges nothing (it was already commented). The reusedexistingvariable stays in scope for the laterif (existing)upsert block. ✅
Issues worth addressing (none block merge)
A. No automated regression tests added. This is a security fix against issue #719, yet there are no existing or new tests for uploadInternalPlugin (only model-level tests exist). The test plan is manual-only. For security work, regression tests would be valuable — at minimum: (1) ../-encoded slug → 400, (2) zip with a symlink entry → 400, (3) upload to another user's slug → 403 and no files written to static/plugin/<slug>/. Recommend adding these.
B. Partial files + file-descriptor leak on rejection. When safeExtractZip rejects mid-archive, previously-written entries remain on disk in pluginPath, and the zipfile handle is never closed (autoClose only fires on end/close, which won't happen since we stop calling readEntry). Consider:
- On reject, call
zipfile.close()(orzipfile.destroy()) to release the fd. - Clean up
pluginPathon error (the controller already has cleanup for the temp upload, but not for a half-extracted plugin dir) so a malicious/valid-then-malicious zip doesn't leave junk behind.
C. Over-broad .. substring check. entry.fileName.includes('..') rejects any entry with two consecutive dots, e.g. version-1..0.txt or my..notes.js. It's safe (over-strict, never under-strict), but a stricter path.normalize/segment-based check would avoid false positives on legitimate filenames. Acceptable trade-off, just noting.
D. Scope creep — dev-stage/.env.dev-stage.sample + .gitignore. These are unrelated to the security fix (#719) and bundle dev-stage environment setup into a security PR. The sample itself is fine (placeholders, not real secrets), but it muddies the security review and should ideally be a separate commit/PR.
E. Minor authorization TOCTOU (low). existing is fetched before extraction; the DB state could change between the ownership check and the later upsertPluginBySlug. Low risk, acceptable.
Cleanup notes
- The trailing-whitespace / blank-line churn at the bottom of
plugin.validation.jsand the reflowedisAdminternaries onto single lines are cosmetic — fine, but they add diff noise.
Verdict: ship it, ideally with (A) regression tests and (B) error-path fd/cleanup added as follow-ups, and (D) split out into its own commit. The core security fixes are correct and well-reasoned.
Reviewed by Claude Code.
Add unit tests exercising the real production code from PR eclipse-autowrx#614: - slug validation rejects path traversal (../, absolute, URL-encoded) - safeExtractZip rejects path-traversal, absolute, and symlink entries, and does not escape the target dir or create symlinks on disk - authorization gate (CWE-862) returns 403 without writing files or mutating the plugin record when the slug is owned by another user Export safeExtractZip from the controller for testability (not used by route handlers). Includes a Python helper to build malicious zip fixtures. Co-Authored-By: Claude <noreply@anthropic.com>
Test note + a finding from running the fixes against real malicious zipsI added regression tests exercising the PR's real production code (commit
Finding while writing the tests: yauzl 3.x already rejects How to run: cd backend
MONGODB_URL="mongodb://localhost:27017/autowrx-test" NODE_ENV=test JWT_SECRET=test-secret \
npx jest tests/unit/controllers/plugin.upload.security.test.js tests/unit/controllers/plugin.upload.auth.test.js --forceExitApplied as a maintainer push to this branch (thanks for enabling edits from maintainers). |
|
Note on the So the red check is pre-existing repo header debt surfaced by the merge-commit checkout, not a regression from this branch. (Unrelated to the security fix — happy to add the missing headers to those main files in a separate PR if helpful, rather than bloating this security PR.) |
Reverts the dev-stage/.env.dev-stage.sample + .gitignore change (commit 3d7aef4) — it is unrelated to the plugin-upload security fix (#719) and should ship in its own PR. Co-Authored-By: Claude <noreply@anthropic.com>
On rejection, safeExtractZip previously leaked the yauzl file descriptor (autoClose only fires on 'end', which never happens after a rejected entry) and left partially extracted files in the plugin directory. Repeated malicious/corrupt uploads would accumulate orphan dirs and exhaust fds. - Close the zipfile fd and destroy in-flight read/write streams on failure - Remove any partially extracted content from targetDir on failure (transactional: full extract or no output) - Always remove the multer temp upload in the caller via try/finally (previously skipped on extraction failure, leaking files under static/uploads) Adds regression tests for the cleanup and that success keeps the target. Co-Authored-By: Claude <noreply@anthropic.com>
|
Resolved review item B ( What changed:
Net effect: failed/corrupt/malicious uploads no longer leak file descriptors or leave orphan plugin dirs / temp files behind, so disk and fd usage stay flat over time. Added regression tests: partial content is removed on rejection, and a successful extraction still keeps the target dir. All 24 plugin-upload regression tests pass locally. Remaining optional follow-ups from the review: C (over-broad |
2 participants
Summary
Fixes three security vulnerabilities in
POST /v2/plugin/upload/:slugreported in issue #719.Vulnerabilities Fixed
Joi.string().required(), allowing URL-encoded../sequences to extract the zip into arbitrary directories (e.g.backend/src/→ RCE on next process restart)slugcustom validator (rejects non-slug characters) + addpath.resolvecontainment check in the controller (defense in depth)spawn('unzip')recreated symbolic links, andexpress.staticfollowed them, allowing read access to any file on disk (e.g..envcontainingJWT_SECRET)spawn('unzip')withyauzl-basedsafeExtractZip()that rejects symlink entries, absolute paths, and../in entry names. Adddotfiles: 'ignore'toexpress.staticmounts for/plugincheckPermissionguard was commented out, and the ownership check ran after extraction had already completedcheckPermissionand redundantauth()from route (auth already applied viarouter.use(auth())at line 26)Files Changed
backend/src/validations/plugin.validation.js— applyslugcustom validator touploadInternal.params.slugbackend/src/controllers/plugin.controller.js— replacespawn('unzip')withsafeExtractZip(), add path containment check, move ownership check before extractionbackend/src/routes/v2/system/plugin.route.js— remove commented-outcheckPermissionand redundantauth()backend/src/app.js— adddotfiles: 'ignore'toexpress.staticmounts for/pluginand/static/pluginbackend/package.json/backend/yarn.lock— addyauzldependencyTest plan
/plugin/<slug>/index.js)../in slug is rejected by validation (e.g...%2F..%2Fsrc→ 400)yarn lintpasses on changed filesCloses #719